feat: add getNumberEvaluation method delegating to getDoubleEvaluation (#501) #1536
Conversation
open-feature#501) Signed-off-by: Carla Köberl <[email protected]>
open-feature#501) Signed-off-by: Carla Köberl <[email protected]>
|
| @@ -287,9 +287,11 @@ private <T> ProviderEvaluation<?> createProviderEvaluation( | |||
| case STRING: | |||
| return provider.getStringEvaluation(key, (String) defaultValue, invocationContext); | |||
| case INTEGER: | |||
| return provider.getIntegerEvaluation(key, (Integer) defaultValue, invocationContext); | |||
| return provider.getNumberEvaluation(key, (Integer) defaultValue, invocationContext); | |||
There was a problem hiding this comment.
do we need to modify/cast here the resolved value to an Integer before returning, for backwards compatibility?
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
Hi, thanks for this contribution! It's a great step towards simplifying the numeric flag evaluation. The overall approach of introducing getNumberEvaluation and deprecating the type-specific ones is correct. I've found a critical issue in the implementation of the new default method where two arguments were swapped, and I've also left a few suggestions to improve the tests to make them more robust and prevent such issues in the future. Keep up the great work!
| return new ProviderEvaluation<>( | ||
| dep.getValue(), | ||
| dep.getReason(), | ||
| dep.getVariant(), | ||
| dep.getErrorCode(), | ||
| dep.getErrorMessage(), | ||
| dep.getFlagMetadata()); |
There was a problem hiding this comment.
There's a bug in the constructor call for ProviderEvaluation. The arguments for variant and reason are swapped. According to the ProviderEvaluation class definition, the constructor expects variant before reason. This will lead to incorrect evaluation details.
| return new ProviderEvaluation<>( | |
| dep.getValue(), | |
| dep.getReason(), | |
| dep.getVariant(), | |
| dep.getErrorCode(), | |
| dep.getErrorMessage(), | |
| dep.getFlagMetadata()); | |
| return new ProviderEvaluation<>( | |
| dep.getValue(), | |
| dep.getVariant(), | |
| dep.getReason(), | |
| dep.getErrorCode(), | |
| dep.getErrorMessage(), | |
| dep.getFlagMetadata()); |
| void noOpNumber() { | ||
| NoOpProvider p = new NoOpProvider(); | ||
| ProviderEvaluation<Number> eval = p.getNumberEvaluation("key", 123456789L, null); | ||
| assertEquals(123456789.0, eval.getValue()); |
There was a problem hiding this comment.
This test is good for verifying the returned value. To make it more robust, I suggest adding assertions for the variant and reason fields as well. This would help catch potential issues like swapped fields in the evaluation details.
assertEquals(123456789.0, eval.getValue());
assertEquals(NoOpProvider.PASSED_IN_DEFAULT, eval.getVariant());
assertEquals(Reason.DEFAULT.toString(), eval.getReason());| @@ -97,6 +100,9 @@ void variant_set() { | |||
| ProviderEvaluation<Integer> int_result = p.getIntegerEvaluation("key", 4, new ImmutableContext()); | |||
| assertNotNull(int_result.getReason()); | |||
|
|
|||
| ProviderEvaluation<Number> number_result = p.getNumberEvaluation("key", 2L, new ImmutableContext()); | |||
| assertNotNull(number_result.getReason()); | |||
There was a problem hiding this comment.
The test method is named variant_set and refers to specification 2.2.4, which is about the variant field. However, the assertion checks getReason(). To align with the test's intent, it should assert on getVariant() instead. I've noticed this pattern for other assertions in this test, but I'm only commenting on the line you've added.
| assertNotNull(number_result.getReason()); | |
| assertNotNull(number_result.getVariant()); |
Please give me some feedback. This is my second open source contribution and I'm not sure if this is the correct approach.
fixes: #501